fix(detail): distinguish "in approval (editable)" from locked, and stop losing write warnings - #2914
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
baozhoutao
force-pushed
the
fix/issue-3794-approval-band
branch
from
July 28, 2026 09:06
be19740 to
22ba5af
Compare
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
…op losing write warnings (framework#3794) The record page told the user the opposite of what the backend does, in both directions. The approval band rendered "Locked for approval" on any pending request, ignoring the node's `lockRecord`. A `lockRecord: false` node is pending WITHOUT locking — that is how an approver amends a record while deciding on it — so the band hid the feature; meanwhile the header Edit button stayed enabled on a genuinely locked record, letting the user fill a form the save would reject with RECORD_LOCKED. `useRecordApprovals` now reads `locks_record` off the pending request (framework#3794), `RecordDetailView` derives `approvalPending` and `approvalLocked` separately and threads both through `InlineEditProvider`, and the band picks the amber lock or a neutral "In approval (editable)" from the host's signals. With no approvals-aware host the `approval_status` fallback is unchanged and still assumes a lock. The write-warning toast (framework #3431/#3455) never fired on `batchTransaction`, which is the record form's save path for a master-detail record — precisely where a user edits a `readonlyWhen`-locked field. It now emits per event, resolving each back to its operation via the response `index`. The toast is localized (en + zh) and worded by reason: a `readonly_when` strip says the field is not editable in this record's current state, not that it is read-only. Finally, the user-state adapter no longer hand-stamps the server-managed `updated_at`, which the server strips and reports — that fired a "Some fields were not saved" warning about a field no user touched, on ordinary page loads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…amework#3794) The full ten-pack parity ratchet (#2872) landed after this branch was cut, so the six new `detail.*` keys existed only in en/zh. Translated into ja, ko, de, fr, es, pt, ru and ar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xuyushun441-sys
pushed a commit
to objectstack-ai/objectstack
that referenced
this pull request
Jul 28, 2026
…ock_record` main shipped the same feature first. #3815 (`a6c3f3806`, #3814/objectui#2902) landed `ApprovalRequestRow.lock_record`, computed `cfg?.lockRecord !== false` off the `node_config_json` snapshot — byte-for-byte the rule this branch's `locks_record` computes, from the same snapshot, with the same default. Only the test file collided textually. `approval-service.ts` and the spec contract auto-merged *clean*, which is the dangerous part: the result declared both fields, so `GET /api/v1/approvals/requests` would have shipped one policy under two names and every client would have had to guess which to read. Resolved toward main: `lock_record` is already merged and released in a changeset, so dropping it would be a breaking change layered on a duplicate. Removed from this branch: - `locks_record` on `rowFromRequest` (plugin-approvals) - `locks_record?: boolean` on `ApprovalRequestRow` (spec contract) - its 3 tests — main's 3 `lock_record` tests are a superset, covering `openNodeRequest` / `listRequests` / `getRequest` on all three cases - the `locks_record` half of the changeset, which main's `approvals-expose-lock-record.md` already describes The changeset is rewritten to the half that is still this branch's own (`droppedFields` on `POST /batch`) and renamed to match; `plugin-approvals` comes off its bump list, because after this resolution the branch no longer touches that package. #3835 (batch creates through the create ingress) is untouched and keeps its own changeset. Console follow-up, NOT covered here: objectstack-ai/objectui#2914 is still open and reads `locks_record`. It must be repointed at `lock_record` or the band falls back to "assume locked" — the exact bug #3794 filed. Verified after resolution: build 71/71 · spec 6836 · rest 419 (incl. all 5 new batch tests) · plugin-approvals 318 (incl. main's 3 `lock_record` tests) · metadata-protocol 71 · no generated-artifact drift. Co-Authored-By: Claude <noreply@anthropic.com>
The approval-band half of this PR landed independently as #2906 while it was open, so the merge is mostly a de-duplication. Resolved toward main for everything #2906 already shipped, keeping only what is genuinely new here. Taken from main wholesale: - `useRecordApprovals` — main's `lock_record` + `recordLockedByApproval`. This PR's field name `locks_record` was simply wrong: framework#3834 merged as `lock_record`, which is what the server actually sends. - `InlineEditContext` — identical `approvalPending` contract; no diff left. - the band's markup/styling (sky "In approval · editable" variant) and the `approvalPendingEditable` / `approvalPendingTooltip` key names, dropping this PR's `inApprovalEditable*` spelling of the same two strings. Kept from this branch: - `disabled: approvalLocked` on the header Edit CTA. #2906 gated inline edit but left the full-form CTA live on a locked record, so the user still filled a screen before Save returned RECORD_LOCKED. - `DetailView` no longer OR-s the `approval_status` mirror into `isLocked` unconditionally. A flow configuring an `approvalStatusField` mirrors `approval_status: 'pending'` regardless of `lockRecord`, so on a `lockRecord: false` node main's derivation re-locked the band while the host had correctly resolved "not locked" — pencils live and saves landing under a band reading "Locked for approval". `approvalPending && !locked` is the tell that the host has an opinion (the provider defaults `approvalPending` to `locked`, so a pre-#2902 host can never produce it). Covered by a new test. - the recall tooltip's unlocked variant, and everything in parts 2 and 3 (batch `droppedFields` warnings, the localized by-reason toast, and dropping the hand-stamped `updated_at`), which main never touched. Also drops a duplicate `Clock` import git merged in from both sides. Green: 2849 tests across plugin-detail / data-objectstack / i18n / react / app-shell; `pnpm type-check` clean on all 76 packages; 0 lint errors.
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Console half of objectstack-ai/objectstack#3794 (framework half: objectstack-ai/objectstack#3834, which adds the
locks_recordand batchdroppedFieldsthis PR consumes).The record page told the user the opposite of what the backend does, in both directions.
1. The band claimed a lock the server does not enforce
"Locked for approval" rendered on any pending request, ignoring the node's
lockRecord. A node declaringlockRecord: falseis pending without locking — the server's hook lets the write through on purpose, so the approver can amend the record as part of deciding on it — and the band told them not to bother. The mirror image was just as bad: on a genuinely locked record the header Edit button stayed live, so the user filled a whole form before the save came backRECORD_LOCKED.The pending request is now authoritative for both signals:
useRecordApprovalsreadslocks_record(resolved server-side from the same node-config snapshot the lock hook reads);RecordDetailViewderivesapprovalPendingandapprovalLockedseparately and threads both throughInlineEditProvider(newapprovalPendingprop);disabledon a locked record — visible-but-off, matching the house style for "the affordance exists and is off".With no approvals-aware host (bare / legacy
DetailView), the oldapproval_status-field fallback is unchanged and still assumes a lock — the safe direction.2. A stripped field vanished silently on the record form's save path
The adapter emitted a write-warning for
create/updateresponses carryingdroppedFields, but not forbatchTransaction— which is how the record form saves a master-detail record, i.e. the one surface where a user actually edits areadonlyWhen-locked field.batchTransactionnow emits one warning per event, resolving each back to its operation via the response'sindex.The toast itself was hardcoded English and called every strip "read-only". It is now localized (
detail.writeStripped*, en + zh) and worded by reason:readonly_whensays the field is not editable in this record's current state, which is what actually happened — the field is editable in other states and the form rendered it as an ordinary input, so "read-only" sends the user hunting for a permission problem that does not exist.3. …and it stopped crying wolf
createObjectStackUserStateAdapterhand-stamped the server-managedupdated_aton every recents/favorites write. The server strips it and reports the strip, so the Console popped "Some fields were not saved" about a field no user ever touched — on ordinary page loads, drowning the signal the toast exists for. It no longer sends the column; the server stamps it anyway.Tests
New: 4 in
DetailView.approvalBand.test.tsx(editable variant; host signals beat theapproval_statusmirror; the lock variant survives; no band when the host reports nothing pending), 2 inonWriteWarning.test.ts(batch events resolve to their op; clean/malformed batches stay quiet), 1 inuserState.test.ts(never sendsupdated_at).Green: 931 tests across
plugin-detail/react/i18n/data-objectstack/app-shell;tscclean on every touched package.Browser-verified against the framework showcase
lockRecord: false), pendinglockRecord: true), pendingRECORD_LOCKED)状态 → Paid+税率changed in one save🤖 Generated with Claude Code